Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Initial implementation of EngineGetPayloadV2 #4833

Merged
merged 8 commits into from
Dec 21, 2022
Merged

Initial implementation of EngineGetPayloadV2 #4833

merged 8 commits into from
Dec 21, 2022

Conversation

Gabriel-Trintinalia
Copy link
Contributor

@Gabriel-Trintinalia Gabriel-Trintinalia commented Dec 16, 2022 •
edited
Loading

Signed-off-by: Gabriel Trintinalia gabriel.trintinalia@gmail.com

PR description

Initial implementation of EngineGetPayloadV2

  • Same result as V1 but adds the block value to the payload:
  • Calculate Block Value currently does not have an implementation and will return 0
{
    "jsonrpc": "2.0",
    "id": 67,
    "result": {
        "executionPayload": {
            "parentHash": "0x3b8fb240d288781d4aac94d3fd16809ee413bc99294a085798a589dae51ddd4a",
            "feeRecipient": "0xaa00000000000000000000000000000000000000",
            "stateRoot": "0xca3149fa9e37db08d1cd49c9061db1002ef1cd58db2210f2115c8c989b2bdf45",
            "logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
            "prevRandao": "0xff00000000000000000000000000000000000000000000000000000000000000",
            "gasLimit": "0x1c9c380",
            "gasUsed": "0x0",
            "timestamp": "0x50",
            "extraData": "0x",
            "baseFeePerGas": "0x7",
            "transactions": [],
            "blockNumber": "0x1",
            "blockHash": "0xae199cf83e812e4eb14e9597ce82b743c01e494c6fe660eab144b0a97e9fdb4d",
            "receiptsRoot": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421"
        },
        "blockValue": "0x0"
    }
}

Fixed Issue(s)

See ethereum/execution-apis#307

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

@Gabriel-Trintinalia
Initial implementation of EngineGetPayloadV2
5a3ed04 
Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>
@Gabriel-Trintinalia Gabriel-Trintinalia added mainnet TeamRevenant GH issues worked on by Revenant Team labels Dec 18, 2022
@Gabriel-Trintinalia Gabriel-Trintinalia marked this pull request as ready for review December 18, 2022 23:06
@Gabriel-Trintinalia Gabriel-Trintinalia self-assigned this Dec 18, 2022
siladu
siladu reviewed Dec 20, 2022
View reviewed changes
...va/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadV2.java Outdated Show resolved Hide resolved
...va/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadV2.java Outdated
@Override
protected JsonRpcResponse createResponse(final JsonRpcRequestContext request, final Block block) {
return new JsonRpcSuccessResponse(
request.getRequest().getId(), blockResultFactory.enginePayloadTransactionCompleteV2(block));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hoping we can make a backwards compatible V2 so that the only thing that needs to know about V2 is the very thinnest layer that maps RPC name to class.

Maybe we could pass in an includeBlockValue or isShanghai to trigger this behaviour in the shared layer below

siladu added a commit to siladu/besu that referenced this pull request Dec 20, 2022
@siladu
Add blockValue to EngineGetPayloadResultV2
8aa0c42 
Ported code over from hyperledger#4833

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@siladu siladu mentioned this pull request Dec 20, 2022
Closed
siladu added a commit to siladu/besu that referenced this pull request Dec 20, 2022
@siladu
Add blockValue to EngineGetPayloadResultV2
46f4390 
Ported code over from hyperledger#4833

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@Gabriel-Trintinalia
Classes reorganization
41392c4 
Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>
@Gabriel-Trintinalia
Classes reorganization
a475252 
Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>
@Gabriel-Trintinalia
Rename class to conform with Java conventions
7f8104c 
Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>
siladu
siladu reviewed Dec 20, 2022
View reviewed changes
...main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/results/BlockResultFactory.java
return new EngineGetPayloadResultWithdraws(block.getHeader(), txs, Quantity.create(blockValue));
}

private long calculateBlockValue(final List<String> ignored) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to separate out for later implementation 👍

...g/hyperledger/besu/ethereum/api/jsonrpc/internal/results/AbstractEngineGetPayloadResult.java Outdated
*/
package org.hyperledger.besu.ethereum.api.jsonrpc.internal.results;

public abstract class AbstractEngineGetPayloadResult {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure there's any benefit to having this Abstract class

.../hyperledger/besu/ethereum/api/jsonrpc/internal/results/EngineGetPayloadResultWithdraws.java Outdated
"executionPayload",
"blockValue",
})
public class EngineGetPayloadResultWithdraws extends AbstractEngineGetPayloadResult {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably contradicts what I said yesterday, but I'm thinking V2 in the name makes more sense than Withdrawals, although I still don't think V2 should leak further than the API level.

Although this object will end up have Withdrawals in it, including withdrawals could and maybe will be data driven (due to presence of Optional withdrawals).
In this PR, the trigger for this different class structure to be built is really blockValue, but I don't think EngineGetPayloadResultBlockValue makes sense either.

Probably better we keep the code close to the spec in this regard, at least for now until it's clear how the API will evolve with V3

.../hyperledger/besu/ethereum/api/jsonrpc/internal/results/EngineGetPayloadResultWithdraws.java Outdated
return blockValue;
}

public static class PayloadResult {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

...va/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadV2.java

import io.vertx.core.Vertx;

public class EngineGetPayloadV2 extends AbstractEngineGetPayload {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is better than extending EngineGetPayload 👍

@Gabriel-Trintinalia
Remove abstract class and rename methods to match Spec (V2)
adcf9a6 
Signed-off-by: Gabriel Trintinalia <gabriel.trintinalia@gmail.com>
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 21, 2022
View reviewed changes
...main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/results/BlockResultFactory.java
return new EngineGetPayloadResultV2(block.getHeader(), txs, Quantity.create(blockValue));
}

private long calculateBlockValue(final List<String> ignored) {

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'ignored' is never used.
siladu
siladu approved these changes Dec 21, 2022
View reviewed changes
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Gabriel-Trintinalia Gabriel-Trintinalia merged commit a483f79 into hyperledger:main Dec 21, 2022
jflo pushed a commit to jflo/besu that referenced this pull request Dec 22, 2022
@Gabriel-Trintinalia @jflo
Initial implementation of EngineGetPayloadV2 (hyperledger#4833)
1b38ddd 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@siladu siladu mentioned this pull request Dec 22, 2022
Closed
macfarla pushed a commit to jflo/besu that referenced this pull request Jan 10, 2023
@Gabriel-Trintinalia @macfarla
Initial implementation of EngineGetPayloadV2 (hyperledger#4833)
51fe6c1 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@siladu siladu mentioned this pull request Jan 14, 2023
Closed
@Gabriel-Trintinalia Gabriel-Trintinalia deleted the engine-get-payload-v2-method branch January 16, 2023 16:18
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
@Gabriel-Trintinalia @eum602
Initial implementation of EngineGetPayloadV2 (hyperledger#4833)
c7568e4 
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siladu siladu siladu approved these changes

Assignees

@Gabriel-Trintinalia Gabriel-Trintinalia

Labels
mainnet TeamRevenant GH issues worked on by Revenant Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Gabriel-Trintinalia @siladu